Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Install phpunit via vendor bin #40678

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Sep 28, 2023

Summary

This means we can all run the same version. And when switching to stableX it can pull the corresponding version too. Also local dev envs and CI run the same phpunit.

Set to draft until I know if this actually works. It works.

TODO

  • Add PHPUnit 9 in a new composer bin
  • Add composer scripts to run main test configuration and files_external test configuration
  • Migrate github workflow scripts to use pinned phpunit instead of a global one

Follow-ups

  • Migrate autotest.sh

Checklist

@ChristophWurst ChristophWurst added the 2. developing Work in progress label Sep 28, 2023
@ChristophWurst ChristophWurst self-assigned this Sep 28, 2023
@ChristophWurst
Copy link
Member Author

alright. this seems to work. only one test fails because I forgot to adjust the working directory.

@blizzz @come-nc @icewind1991 @kesselb @nickvergessen any objections here?

my personal stretch goal is to standardize running phpunit tests through a composer script. I'm mostly annoyed by the slow and cumbersome dav unit tests. what I want is a simple and fast composer run test:dav:unit that runs all true unit tests of (cal)dav with the locked phpunit version and no separate installation.

for those who never run tests locally there will be no change for now.

"config": {
"sort-packages": true,
"platform": {
"php": "8.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8.1 possible?
See #40636

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Once master doesn't work with 8.0 anymore we can bump the platform here 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the point of #40636 is to update the CI before dropping 8.0.

Because I fear dropping 8.0 will be decided late in the release cycle.

We have a php lint step that checks all supported versions, it’s not needed for phpunit to run on the oldest one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I drop the platform requirement here, composer will pull sub dependencies of phpunit that are incompatible with PHP8.0. So CI won't pass. It's really just about one incompatible package. There is an older version that works with 8.0 as well, which is installed now.

IMO dropping PHP8.0 support and removing it from CI is an atomic step. Either master doesn't support 8.0 and we don't test it, or it still supports it and we have tests. Dropping 8.0 CI now is a risk if 8.0 support is not removed in the this release cycle.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 2, 2023
@ChristophWurst ChristophWurst marked this pull request as ready for review October 2, 2023 08:30
This means we can all run the same version. And when switching to
stableX it can pull the corresponding version too. Also local dev envs
and CI run the same phpunit.

Signed-off-by: Christoph Wurst <[email protected]>
@ChristophWurst ChristophWurst force-pushed the chore/phpunit-vendor-bin branch from 0952d56 to 0e64ec6 Compare October 2, 2023 09:33
@ChristophWurst ChristophWurst merged commit cd1d980 into master Oct 2, 2023
47 checks passed
@ChristophWurst ChristophWurst deleted the chore/phpunit-vendor-bin branch October 2, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants